Skip to content

Avoid allocating the CandidateSet when there's a single match - #9622

Merged
davidfowl merged 3 commits into
masterfrom
davidfowl/less-alloc
Apr 22, 2019
Merged

Avoid allocating the CandidateSet when there's a single match#9622
davidfowl merged 3 commits into
masterfrom
davidfowl/less-alloc

Conversation

@davidfowl

@davidfowl davidfowl commented Apr 21, 2019 •
edited
Loading

Copy link
Copy Markdown
Member
  • Add fast path for 0 route values, and policies

Scenario, hello world plaintext (the /plaintext sample in the RoutingSandbox) hit with 10K requests.

Allocations before:

image

Allocations after:

image

Microbenchmarks:

MatcherSingleEntryBenchmark

Before:

           Method |     Mean |     Error |    StdDev |        Op/s | Scaled | ScaledSD |  Gen 0 | Allocated |
----------------- |---------:|----------:|----------:|------------:|-------:|---------:|-------:|----------:|
         Baseline | 106.5 ns |  1.762 ns |  1.472 ns | 9,387,641.5 |   1.00 |     0.00 | 0.0074 |      40 B |
              Dfa | 185.6 ns |  3.724 ns |  8.556 ns | 5,388,922.3 |   1.74 |     0.08 | 0.0212 |     112 B |
 LegacyTreeRouter | 954.0 ns | 20.355 ns | 38.231 ns | 1,048,254.6 |   8.96 |     0.37 | 0.0458 |     408 B |
     LegacyRouter | 711.2 ns | 13.520 ns | 11.985 ns | 1,406,107.4 |   6.68 |     0.14 | 0.0525 |     280 B |

After:

           Method |      Mean |      Error |     StdDev |         Op/s | Scaled | ScaledSD |  Gen 0 | Allocated |
----------------- |----------:|-----------:|-----------:|-------------:|-------:|---------:|-------:|----------:|
         Baseline |  96.00 ns |  0.8115 ns |  0.7193 ns | 10,416,201.9 |   1.00 |     0.00 | 0.0076 |      40 B |
              Dfa |  97.07 ns |  1.0586 ns |  0.9902 ns | 10,301,416.6 |   1.01 |     0.01 |      - |       0 B |
 LegacyTreeRouter | 913.16 ns | 17.9336 ns | 17.6132 ns |  1,095,100.5 |   9.51 |     0.19 | 0.0772 |     408 B |
     LegacyRouter | 679.49 ns | 10.7855 ns | 10.0888 ns |  1,471,691.7 |   7.08 |     0.11 | 0.0534 |     280 B |

@davidfowl
Avoid allocating the CandidateSet when there's a single match
337fcae 
- Add fast path for 0 route values, and policies
@davidfowl
davidfowl requested review from JamesNK and rynowak April 21, 2019 23:13
JamesNK
JamesNK reviewed Apr 21, 2019
View reviewed changes
Comment thread src/Http/Routing/src/Matching/DfaMatcher.cs Outdated
Comment thread src/Http/Routing/src/Matching/DfaMatcher.cs Outdated
rynowak
rynowak reviewed Apr 21, 2019
View reviewed changes
Comment thread src/Http/Routing/src/EndpointSelectorContext.cs
public RouteValueDictionary RouteValues
{
get => _routeValues;
get => _routeValues ?? (_routeValues = new RouteValueDictionary());

@rynowak rynowak Apr 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to do this, we can take advantage of it in more cases, including the ones that involve cnadidate set.

@JamesNK JamesNK Apr 21, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is beyond the scope of this PR, but what about reusing RVD instances? It would be similar to how we're reusing HttpContext et al. It's basically a per-request allocation that is tied to the route values feature.

Reusing RVD, and the routing feature, would save allocations on every request, not just those with no route values.

Caching per-request objects on reused HttpContexts

@davidfowl davidfowl Apr 22, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just straight up do this in Kestrel I fear we won't be able to do the more generic caching for 3.0

@davidfowl davidfowl Apr 22, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to do this, we can take advantage of it in more cases, including the ones that involve cnadidate set.

Yes, we can, I just wanted to minimize the code change for now. I have to look at see what things are assuming route values are never null when looking at CandidateState.RouteValues (maybe I can do it statiaclly with #enable nullable 😄 )

@rynowak rynowak Apr 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CandidateSet has some special semantics around RVD that should just be removed. If treats null RVD as a shorthand for not-a-match in some APIs.

@rynowak

rynowak commented Apr 21, 2019

Copy link
Copy Markdown
Contributor

####?s

@davidfowl

davidfowl commented Apr 22, 2019

Copy link
Copy Markdown
Member Author

####?s

Will get some. I went awaty but now I'm back

rynowak
rynowak reviewed Apr 22, 2019
View reviewed changes
Comment thread src/Http/Routing/src/Matching/DfaMatcher.cs
_selector = selector;
_states = states;
_maxSegmentCount = maxSegmentCount;
_isDefaultEndpointSelector = selector is DefaultEndpointSelector;

@rynowak rynowak Apr 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd feel slightly better if this was an exact type equality check. This isn't public, but that would capture the intent better.

@davidfowl davidfowl Apr 22, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn’t it sealed? Future proofing?

@Tratcher
Tratcher removed their request for review April 22, 2019 04:56
@davidfowl
davidfowl merged commit d62d33c into master Apr 22, 2019
@davidfowl
davidfowl deleted the davidfowl/less-alloc branch April 22, 2019 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JamesNK JamesNK JamesNK approved these changes
@analogrelay analogrelay Awaiting requested review from analogrelay
@jkotalik jkotalik Awaiting requested review from jkotalik
+1 more reviewer
@rynowak rynowak rynowak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@davidfowl @rynowak @JamesNK